Skip to content
This repository has been archived by the owner on Nov 15, 2023. It is now read-only.

Improve subkey error reporting #4504

Merged
merged 1 commit into from
Dec 27, 2019
Merged

Improve subkey error reporting #4504

merged 1 commit into from
Dec 27, 2019

Conversation

NikVolf
Copy link
Contributor

@NikVolf NikVolf commented Dec 26, 2019

     Running `target/debug/subkey generate --words 1234123`

Error:
        Invalid number of words given for phrase: must be 12/15/18/21/24

@NikVolf NikVolf added the A0-please_review Pull request needs code review. label Dec 26, 2019
@parity-cla-bot
Copy link

It looks like @NikVolf signed our Contributor License Agreement. 👍

Many thanks,

Parity Technologies CLA Bot

Formatted(String),
}

fn execute_proc<C: Crypto>(matches: ArgMatches)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just use fn main() -> Result<(), Error>

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It does not really pretty-print error this way, see nv-sk-err-msin branch

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If you do this:

impl fmt::Debug for Error {

The error message should look good.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Better, but still worse than here if you ask me (updated nv-sk-err-msin branch)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The only different I see is that there is no new line after Error: in the output? I think hat is okay and less code is always better, IMHO^^

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@bkchr updated

@NikVolf NikVolf force-pushed the nv-sk-err branch 2 times, most recently from 532eb15 to aad663e Compare December 27, 2019 09:06
Copy link
Member

@bkchr bkchr left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good, ty!

One last nitpick :)

@@ -270,7 +270,24 @@ fn get_uri(match_name: &str, matches: &ArgMatches) -> String {
}
}

fn execute<C: Crypto>(matches: ArgMatches)
#[derive(derive_more::Display, derive_more::From)]
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you please change the function above to return an error as well? (get_uri)

@bkchr bkchr merged commit 0fcb22c into master Dec 27, 2019
@bkchr bkchr deleted the nv-sk-err branch December 27, 2019 20:07
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
A0-please_review Pull request needs code review.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants